Skip to content

fix: remove redundant review rules and fix strategy/criteria mismatch - #348

Merged
nhorton merged 7 commits into
mainfrom
review-rule-fixes
Apr 9, 2026
Merged

fix: remove redundant review rules and fix strategy/criteria mismatch#348
nhorton merged 7 commits into
mainfrom
review-rule-fixes

Conversation

@nhorton

@nhorton nhorton commented Apr 8, 2026

Copy link
Copy Markdown
Contributor

Summary

Several review cleanups driven by the observation that the add_documentation_rules step review was taking forever to run:

  • job.yml — fix strategy/criteria mismatch: Change add_documentation_rules review from strategy: individual to strategy: matches_together. Two of its three criteria (Documentation Covered, Efficient Rule Count) are project-wide and cross-cutting, so running them per-file forced each spawned reviewer to redo the same full project scan N times.
  • job.yml — drop duplicated structural check: Remove the apply_rule "Valid Configuration" criterion. The deepreview DeepSchema already enforces structural/schema validation via json_schema_path.
  • job.yml — consolidate duplicated rule YAML: add_deepwork_native_reviews previously showed the same two rules twice (once as a file-ref reference example, once as the inline version to emit) with contradictory guidance about which form to use. Collapsed to a single copy.
  • Delete src/deepwork/standard_jobs/deepwork_reviews/.deepreview (job_definition_review rule): fully redundant with the job_yml DeepSchema which already targets the same files and covers the same structural/coherence checks.
  • Delete job_schema_instruction_compatibility from the top-level .deepreview: its entire purpose was verifying job.yml files conform to job.schema.json, which is exactly what the job_yml DeepSchema's json_schema_path already does.
  • Add review-strategy-matches-criteria-scope requirement to the job_yml DeepSchema so this class of bug is caught going forward: review strategy: individual must only be used when every criterion can be evaluated from a single file in isolation. Cross-file / set-level criteria must use matches_together.

Test plan

  • All YAML files parse cleanly (python3 -c "import yaml; yaml.safe_load(...)")
  • Review unit tests pass (uv run pytest tests/unit/review/ — 236 passed)
  • CI checks
  • add_documentation_rules step review completes in reasonable time on next discover_rules run

🤖 Generated with Claude Code

nhorton and others added 2 commits April 8, 2026 17:16
Several review cleanups driven by the observation that the `add_documentation_rules` step review was taking forever:

- job.yml: change `add_documentation_rules` review from `strategy: individual` to `strategy: matches_together`. Two of its three criteria (Documentation Covered, Efficient Rule Count) are project-wide and cross-cutting, so running them per-file forced each reviewer to redo the full project scan N times.
- job.yml: drop the `apply_rule` "Valid Configuration" criterion — the `deepreview` DeepSchema already enforces structural/schema validation via `json_schema_path`.
- job.yml: collapse the duplicated rule-YAML blocks in `add_deepwork_native_reviews`. The step previously showed the same two rules twice (once as a file-ref reference example, once as the inline version to emit) and told the agent to consult both, creating a contradiction about which form to use.
- Delete `src/deepwork/standard_jobs/deepwork_reviews/.deepreview` (`job_definition_review` rule): fully redundant with the `job_yml` DeepSchema which covers the same files and criteria.
- Delete `job_schema_instruction_compatibility` from the top-level `.deepreview`: its whole purpose was verifying job.yml files conform to `job.schema.json`, which is exactly what the `job_yml` DeepSchema's `json_schema_path` already enforces.
- Add `review-strategy-matches-criteria-scope` requirement to the `job_yml` DeepSchema so this class of bug is caught going forward: review `strategy: individual` must only be used when every criterion can be evaluated from a single file in isolation.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Discourages the sentinel-placeholder-file workaround (e.g., writing a NONE.md
file when the real result is "nothing to report") used to satisfy required
file_path outputs that would otherwise be empty. The fix is to use type: string
for outputs whose list can legitimately be empty, since string outputs can
naturally carry a narrative "no items found" message.

Kept as SHOULD so genuine filesystem-artifact needs (manifests, etc.) can still
opt into a sentinel, but the common "required output can't be empty" workaround
is explicitly not a sufficient reason.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
nhorton and others added 5 commits April 8, 2026 17:32
The step previously directed the agent to write a .deepwork/tmp/migrated_skills/NONE.md
sentinel file as a workaround for "required file_path output can't be empty" — exactly
the anti-pattern the new no-sentinel-files DeepSchema requirement added in a4dc74c
prohibits. Verified the MCP validation accepts empty lists for required file_path
outputs (no minimum-length check in src/deepwork/jobs/mcp/tools.py), so the sentinel
workaround was never necessary.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The previous consolidation commit (1994f94) removed the duplicated rule-YAML
blocks but also removed the pointers to plugins/claude/example_reviews/*.md,
which contain the full detailed prompts that the inline YAML condenses. Those
pointers are legitimately useful — they tell an agent writing condensed inline
versions where to read for tone, intent, and checklist context.

Restored as a "Reference material" section that clearly resolves the original
inline-vs-file-based contradiction: always emit the inline form (target
projects don't have the example files), and use the example files only as
reference when deciding what the condensed inline text should say.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The previous wording forbade any cross-file reasoning from `strategy:
individual`, which would disqualify legitimate cases like a Python code review
rule where the reviewer may need to consult other project files (imports,
references, related code, conventions) while evaluating a single changed file.

Reworded so the distinction is about whether the reviewer needs to look at the
OTHER CHANGED FILES in the same review set, not whether they need to look at
any other file at all. Individual is appropriate when focus is heavily on each
file and the reviewer is unlikely to need to see the other changed files;
matches_together is for criteria that reason about the set as a whole
(coverage, deduplication, consistency across the set).

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Restructured the requirement to use SHOULD/MAY/MUST NOT explicitly, matching
the RFC 2119 style of the other requirements in this schema. The substantive
guidance is unchanged: individual is preferred when focus is on each file and
the reviewer is unlikely to need to see the other changed files (consulting
unchanged project files is explicitly allowed as MAY); matches_together is
preferred when changed files need to be looked at alongside each other; and
individual is a hard MUST NOT when any criterion is a property of the set of
changed files as a whole.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The meta-DeepSchema governing deepschema files previously only matched
.deepwork/schemas/*/deepschema.yml and **/.deepschema.*.yml, which missed
the project's own standard schemas in src/deepwork/standard_schemas/ —
they were exempt from the rules they define for users.

Consolidated both hard-coded directory matchers into a single generic
**/deepschema.yml pattern that:
- Covers .deepwork/schemas/*/deepschema.yml (user-created named schemas)
- Covers src/deepwork/standard_schemas/*/deepschema.yml (built-in schemas)
- Covers any other location a user might put a deepschema.yml
- Self-matches the meta-schema itself, which is fine — it conforms to
  its own requirements

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@nhorton
nhorton added this pull request to the merge queue Apr 8, 2026
Merged via the queue into main with commit b872dc9 Apr 9, 2026
5 checks passed
@nhorton
nhorton deleted the review-rule-fixes branch April 9, 2026 00:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant